RFC: Add scheduling profiler to DevTools #19892

taneliang · 2020-09-23T13:54:36Z

Summary

This PR adds the scheduling profiler as a new tab/panel to DevTools. I'm not sure if there are better approaches though; feedback welcome!

The main goal of integrating is to allow us to add a record button to record a performance profile in a future PR. I've figured out a way to do it, and I've made a POC here: https://github.com/taneliang/react-scheduling-profiler-devtools-integration-poc.

The scheduling profiler is hidden behind a new enableIntegratedSchedulingProfiler DevTools feature flag. Apart from a bundle size increase (main.js grows from 1.22 MiB to 1.77 MiB), this PR shouldn't affect any functionality in the existing DevTools if the feature flag is off.

This PR is nearly complete, but I'm not sure if adding a tab is what you have in mind @bvaughn. I'm also aware that React 17 will be released soon, and I'm not sure if you'll want to wait until after that release. If this approach looks good and we want to proceed, I'll finish the remaining tasks :)

Notes

Scheduling profiler dark mode isn't disabled in the shell and core packages, even if the dark mode flag is set to false. This is happening because those packages don't mount the scheduling profiler in a different document. As the scheduling profiler has some usability issues in dark mode (from the minor (always-white canvas background), to the major (invisible white tooltip text on a white background)), this likely blocks the scheduling profiler from being released as an integrated part of DevTools. Although we can fix this now by switching the DevTools to light mode when the user opens the scheduling profiler tab, I think it'll be better to just fix the dark mode colors. I/we can do this in future PRs.
main.js bundle size increases from 1.22 MiB to 1.77 MiB. We can trim this to 1.67 MiB if we remove the profilerBrowser.png asset.
The image causes the scheduling profiler to scroll in the shell package.

TODO

Ensure Edge extension works (it probably works but I haven't tested it).
Fix context menu dismissal behavior.

Future work

Implement profile record button
Replace scheduling profiler tab icon (it's currently the same one as the Profiler tab)
Add Settings modal to non-standalone deployment of scheduling profiler

Test Plan

Tested Chrome and Firefox extensions.
Tested Electron app
Tested shell
Flipped enableIntegratedSchedulingProfiler to false and ensured scheduling profiler tab/panel does not appear.

FF issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1294996

…oolsFeatureFlags

…rofiler

…ode=false

codesandbox-ci · 2020-09-23T13:57:14Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bc1a8e6:

Sandbox	Source
React	Configuration

taneliang · 2020-09-23T14:02:52Z

packages/react-devtools-extensions/firefox/manifest.json

@@ -32,7 +32,7 @@

  "devtools_page": "main.html",

-  "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'",
+  "content_security_policy": "script-src 'self' 'unsafe-eval' blob:; object-src 'self'",


This may be a little unsafe, but it's necessary if we want to load our worker as a blob. https://bugzilla.mozilla.org/show_bug.cgi?id=1294996

sizebot · 2020-09-23T14:03:04Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against bc1a8e6

sizebot · 2020-09-23T14:05:03Z

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against bc1a8e6

As this PR added the url-loader package to 2 other packages, Yarn hoisted it to the root node_modules folder. However, because file-loader was only a dependency of the scheduling profiler package, it was located in the scheduling profiler's node_modules folder. Thus, during the build process, url-loader could not find file-loader.

bvaughn · 2020-09-23T20:40:08Z

Hey that's neat. I didn't realize you were interested in working on this 😄 Happy to see it though!

I don't think I'm comfortable merging this new tab into the DevTools extension until the integration is further along. But I'd be happy to review and discuss things as we go. Want to make a new fork– or just pick a branch other than master– and target new PRs to it?

taneliang · 2020-09-24T10:34:11Z

Apologies for the surprise, the idea of using the Chrome DevTools Protocol was just roasting in my head and I had to get it out 😆

I don't think I'm comfortable merging this new tab into the DevTools extension until the integration is further along. But I'd be happy to review and discuss things as we go. Want to make a new fork– or just pick a branch other than master– and target new PRs to it?

This makes sense! I'm good with either. We may want to better define what we want to achieve though, so that I can work towards that and maybe hand it off to you at some point. Here's what I had in mind:

Integrate the scheduling profiler into DevTools so that we have a base to work on (will be fulfilled by this PR)
Add that profile record button as an MVP that allows users to use the scheduling profiler more easily. This will still require a custom build of React with enableSchedulingProfiler turned on though.
Somehow allow profiling of apps without requiring enableSchedulingProfiler? This will allow profiling of deployed apps like the React Profiler, but it looks like this will require quite a bit of work to integrate it into the bridge and stuff.

I'm also happy to drop this if this isn't something we want to work towards.

bvaughn · 2020-09-24T13:12:10Z

Those goals sound good at a high level.

I'll want to play with the UI a little probably, but that can be done at any point.

Detecting profiling support

We will probably want to add a new boolean prop that the reconciler injects into DevTools that lets it know if this advanced profiling mode is supported. There's some precedent here for regular profiling.

The DevTools backend detects if Profiling is supported by inspecting a Fiber:

react/packages/react-devtools-shared/src/backend/renderer.js

Line 1135 in a99bf5c

const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration');

And then it notifies the DevTools frontend any time a new root is mounted:

react/packages/react-devtools-shared/src/backend/renderer.js

Lines 1138 to 1141 in a99bf5c

    
           pushOperation(TREE_OPERATION_ADD); 
        
           pushOperation(id); 
        
           pushOperation(ElementTypeRoot); 
        
           pushOperation(isProfilingSupported ? 1 : 0);

The frontend then enables or disables the Profiler record button based on whether any of the current roots can even support profiling. Even if profiling is not supported though, the "import profile" functionality still works.

I think we'd want to do the same for the new profiler– but we wouldn't be able to determine Profiling support by inspecting a fiber. We'd need to explicitly pass a parameter to DevTools telling it whether the feature flag existed and was enabled:

react/packages/react-reconciler/src/ReactFiberReconciler.new.js

Lines 712 to 741 in a99bf5c

    
           export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean { 
        
             const {findFiberByHostInstance} = devToolsConfig; 
        
             const {ReactCurrentDispatcher} = ReactSharedInternals; 
        
             return injectInternals({ 
        
               bundleType: devToolsConfig.bundleType, 
        
               version: devToolsConfig.version, 
        
               rendererPackageName: devToolsConfig.rendererPackageName, 
        
               rendererConfig: devToolsConfig.rendererConfig, 
        
               overrideHookState, 
        
               overrideHookStateDeletePath, 
        
               overrideHookStateRenamePath, 
        
               overrideProps, 
        
               overridePropsDeletePath, 
        
               overridePropsRenamePath, 
        
               setSuspenseHandler, 
        
               scheduleUpdate, 
        
               currentDispatcherRef: ReactCurrentDispatcher, 
        
               findHostInstanceByFiber, 
        
               findFiberByHostInstance: 
        
                 findFiberByHostInstance || emptyFindFiberByHostInstance, 
        
               // React Refresh 
        
               findHostInstancesForRefresh: __DEV__ ? findHostInstancesForRefresh : null, 
        
               scheduleRefresh: __DEV__ ? scheduleRefresh : null, 
        
               scheduleRoot: __DEV__ ? scheduleRoot : null, 
        
               setRefreshHandler: __DEV__ ? setRefreshHandler : null, 
        
               // Enables DevTools to append owner stacks to error messages in DEV mode. 
        
               getCurrentFiber: __DEV__ ? getCurrentFiberForDevTools : null, 
        
             }); 
        
           }

Unified profiles

Longer term, I'd like a single profiling UI with a button that starts Profiling in both React and the browser (if supported) and then aligns the data so the two voices can cross link. That seems like a pretty big lift though, so for now it's okay if we keep them separate.

taneliang · 2020-09-25T08:45:56Z

Thanks for the pointers! I'll take a closer look at them.

In the long term, would we also want to look into recording scheduling profiler data through the existing Profiler's bridge? When I last checked, Safari's exported performance profiles don't contain user timing marks, so the existing approach won't work with Safari.

By the way, I have exams coming up next week so I'll likely only be able to get back to this in 1-2 weeks.

bvaughn · 2020-09-25T12:43:05Z

In the long term, would we also want to look into recording scheduling profiler data through the existing Profiler's bridge?

Not sure what you mean by "through the bridge". I have been imagining these two forms of Profiling will eventually be started/stopped with the same button (although the mechanisms for start/stopping them both will be different).

When I last checked, Safari's exported performance profiles don't contain user timing marks, so the existing approach won't work with Safari.

Safari isn't a supported browser for a few reasons. That's fine.

By the way, I have exams coming up next week so I'll likely only be able to get back to this in 1-2 weeks.

No hurry.

taneliang · 2020-09-25T13:14:45Z

Safari isn't a supported browser for a few reasons. That's fine.

👍

Not sure what you mean by "through the bridge".

Ah sorry. I meant using the same architecture as the Profiler, where the extension's frontend uses a bridge to the backend to transfer profiling data. Correct me if I'm wrong, but from what I can tell, the extension injects a DevTools backend script into the running app, and the backend gathers profiling data before sending it to the extension frontend when profiling stops.

If that's accurate, I was wondering if it would make sense to have the backend gather scheduling profiler data along with the existing Profiler's data, so that:

The scheduling profiler can support Safari (by not relying on User Timing marks).
Users can use the scheduling profiler without needing a custom build of React with the feature flag flipped.

But since we don't intend to support Safari, this suggestion is probably irrelevant. I'd imagine that the second goal can be achieved in an easier way (maybe by having the backend enable advanced profiling data collection at runtime when needed).

I have been imagining these two forms of Profiling will eventually be started/stopped with the same button

This is what I have in mind as well 👍

bvaughn · 2020-09-25T13:22:11Z

I was wondering if it would make sense to have the backend gather scheduling profiler data along with the existing Profiler's data

You're right that we could log this data somewhere else– either through some new React-specific API or the DevTools backend could e.g. override the native performance.mark and performance.measure APIs (or polyfill them in the case of Safari) so as to still collect this data– kind of like we do with the console APIs.

And maybe there are reasons to consider doing this in terms of aligning scheduler profiling data with the pre-existing React Profiler data, but I think supporting Safari is a non-goal. The added context of the call stack flame graph and the non-React user timing marks make the new scheduling profiler much more useful. If anything, I'd like to see us add more such context to the scheduling profiler UI (which would have to come from the browser's profiling data).

taneliang · 2020-09-25T14:24:18Z

👍

Would we want to prioritize displaying React Profiler data in the scheduling profiler, over browser profiling data? I'd imagine that React context will be more useful as that information probably can't be obtained with existing tools, e.g. with a React component flame chart like these old User Timing measures (if possible), you'll be able to tell which components are actually rendering in a particular render measure in the scheduling profiler UI.

bvaughn · 2020-09-25T14:32:53Z

Would we want to prioritize displaying React Profiler data in the scheduling profiler, over browser profiling data?

No, I don't think so. Although it might be helpful to also add the React component stack. 😄

with a React component flame chart like these old User Timing measures (if possible), you'll be able to tell which components are actually rendering in a particular render measure in the scheduling profiler UI.

You can tell that now, with the browser's call stack. It's just a lot more buried (especially if you want to know the component stack of a given component.)

stale · 2020-12-25T13:06:31Z

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

bvaughn · 2021-07-16T15:36:20Z

This PR and the repo here are really great btw. Thanks again for taking the time to put them together. I'm going to pick this work up now and try to get the new profiler integrated. Sorry it's taken so long 🙇🏼

bvaughn · 2021-07-19T21:17:34Z

Closing this in favor of ##21897. Thanks again for getting the ball rolling here though!

taneliang added 14 commits September 23, 2020 11:35

Remove unused scheduling profiler assets

771f777

Bump worker-loader to fix error when using an inline: 'no-fallback'

0812891

Add scheduling profiler to inline DevTools and shell

f9e8129

Fix core build

bf26246

Add scheduling profiler to extensions

d46c447

Fix event listeners listening to wrong iframe document

3f80831

Hide app name except in standalone app

5cd54fb

Add top border to match other DevTools

b08b826

Relax Firefox extension CSP to load worker blob

25334f6

FF issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1294996

Sync scheduling profiler package version with other DevTools packages

5b8cf43

Remove unused DEVTOOLS_VERSION env var from scheduling profiler

aff9466

Move SchedulingProfilerFeatureFlags -> react-devtools-shared/src/DevT…

899cd9a

…oolsFeatureFlags

Gate scheduling profiler tab/panel behind enableIntegratedSchedulingP…

4e68b5f

…rofiler

Force scheduling profiler extension panel light mode if enableSPDarkM…

825ad62

…ode=false

facebook-github-bot added the CLA Signed label Sep 23, 2020

taneliang commented Sep 23, 2020

View reviewed changes

taneliang added 2 commits September 23, 2020 22:11

Fix lint

41b7a3b

stale bot added the Resolution: Stale Automatically closed due to inactivity label Dec 25, 2020

bvaughn mentioned this pull request Jul 13, 2021

[DevTools] Scheduling Profiler umbrella task #21866

Closed

20 tasks

bvaughn added the Component: Developer Tools label Jul 15, 2021

stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 15, 2021

bvaughn changed the base branch from master to main July 15, 2021 00:46

bvaughn mentioned this pull request Jul 16, 2021

[DRAFT] Import scheduling profiler into DevTools Profiler #21897

Merged

10 tasks

bvaughn closed this Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Add scheduling profiler to DevTools #19892

RFC: Add scheduling profiler to DevTools #19892

taneliang commented Sep 23, 2020

codesandbox-ci bot commented Sep 23, 2020 •

edited

Loading

taneliang Sep 23, 2020

sizebot commented Sep 23, 2020 •

edited

Loading

sizebot commented Sep 23, 2020 •

edited

Loading

bvaughn commented Sep 23, 2020

taneliang commented Sep 24, 2020

bvaughn commented Sep 24, 2020

taneliang commented Sep 25, 2020 •

edited

Loading

bvaughn commented Sep 25, 2020

taneliang commented Sep 25, 2020

bvaughn commented Sep 25, 2020

taneliang commented Sep 25, 2020

bvaughn commented Sep 25, 2020

stale bot commented Dec 25, 2020

bvaughn commented Jul 16, 2021

bvaughn commented Jul 19, 2021

RFC: Add scheduling profiler to DevTools #19892

RFC: Add scheduling profiler to DevTools #19892

Conversation

taneliang commented Sep 23, 2020

Summary

Notes

TODO

Future work

Test Plan

codesandbox-ci bot commented Sep 23, 2020 • edited Loading

taneliang Sep 23, 2020

Choose a reason for hiding this comment

sizebot commented Sep 23, 2020 • edited Loading

Size changes (experimental)

sizebot commented Sep 23, 2020 • edited Loading

Size changes (stable)

bvaughn commented Sep 23, 2020

taneliang commented Sep 24, 2020

bvaughn commented Sep 24, 2020

Detecting profiling support

Unified profiles

taneliang commented Sep 25, 2020 • edited Loading

bvaughn commented Sep 25, 2020

taneliang commented Sep 25, 2020

bvaughn commented Sep 25, 2020

taneliang commented Sep 25, 2020

bvaughn commented Sep 25, 2020

stale bot commented Dec 25, 2020

bvaughn commented Jul 16, 2021

bvaughn commented Jul 19, 2021

codesandbox-ci bot commented Sep 23, 2020 •

edited

Loading

sizebot commented Sep 23, 2020 •

edited

Loading

sizebot commented Sep 23, 2020 •

edited

Loading

taneliang commented Sep 25, 2020 •

edited

Loading